Repository navigation
[CHANGE] Reuse the same request session on same instance - #110
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Session cleanup needs a deterministic public API, and the reuse behavior lacks regression coverage.
3 open findings
What changed in this PR
Reuses one HTTP session per DataApi instance to enable connection pooling.
Changes:
- Adds session creation and cleanup.
- Routes requests through the shared session.
- Updates the changelog.
| File | Description |
|---|---|
learnosity_sdk/request/dataapi.py |
Introduces and uses a shared request session. |
ChangeLog.md |
Records session reuse. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pablo-estevez-lrn
changed the title
Add docstring to the close method for clarity. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
michael-linnane-lrn
left a comment
michael-linnane-lrn
left a comment
There was a problem hiding this comment.
Hey @pablo-estevez-lrn, two comments. We need to be careful when altering existing classes since we don't know how the integrations might be used already. Should be easy to fix with backwards compatibility though
| session = getattr(self, '_session', None) | ||
| if session is not None: | ||
| session.close() |
There was a problem hiding this comment.
❓ Question: Why allow later requests? Would we not just fully drop it and have an actual blank slate?
| session = getattr(self, '_session', None) | |
| if session is not None: | |
| session.close() | |
| if getattr(self, '_session', None) is not None: | |
| self._session.close() | |
| self._session = None |
There was a problem hiding this comment.
2 reasons:
- To avoid an invalid state after close(), when we raise an error
None does not have an attribute post.(after the last commit, will be creating a new session instead) - To use the same known semantic that requests, where close is closing the transport, but not converting to a blank slate.
If you want to destroy the object, you can always usedel()
it will be functionally equivalent, so happy to change if you want
There was a problem hiding this comment.
Wouldn't cookies persist through into the reuse?
There was a problem hiding this comment.
Its okay to sink the above btw, just probably worth adding a note in the docstring that certain values will persist even after closing
|
Thanks Pablo. The only overarching comment I have is that |
|
Checklist
Feature
Bug
Security
Documentation
ChangeLog.md updated
Tests added
All testsuites passed
make distcompleted successfullyBump package version if release coming